Skip to content

feat: support ObjectID on the other fields#15

Open
harry-73 wants to merge 10 commits intojamauro:mainfrom
harry-73:main
Open

feat: support ObjectID on the other fields#15
harry-73 wants to merge 10 commits intojamauro:mainfrom
harry-73:main

Conversation

@harry-73
Copy link
Copy Markdown

@harry-73 harry-73 commented Aug 29, 2024

indexedDB does not store ObjectID as ObjectID. So when the package loads the data from indexedDB to minimongo, the original ObjectID is not seemed as ObjectID but plain Object and it creates issue with queries on server side.

@harry-73 harry-73 changed the title feat: support ObjectID on the other field feat: support ObjectID on the other fields Aug 29, 2024
@harry-73
Copy link
Copy Markdown
Author

Hi, is there a chance to have this PR merged?

@jamauro
Copy link
Copy Markdown
Owner

jamauro commented Apr 21, 2025

I'm not opposed to the feature generally. But I don't think this approach is the right solution because it won't work for other nested data. I also think that generally most Meteor devs don't use ObjectIDs so it'd be nice to find a way to support the feature without incurring a performance penalty, ideally also without needing to configuring anything.

@harry-73
Copy link
Copy Markdown
Author

Thanks for your reply. You're right it based of my usecase (referencing others documents from other collections ar top level).
For zero confugiration, an config parameter (false be default) could b added to enable or not this feature. So most of people will have nothing to set up.
For nested data, it must be recursive but we could limit the depth with a config parameter.

@jamauro
Copy link
Copy Markdown
Owner

jamauro commented Apr 23, 2025

Hmm, I don't love the idea of adding multiple configs for this.

One idea would be to apply it if the collection is known to have idGeneration: MONGO and not limit the depth.

Maybe there is an even simpler solution. I haven't looked at how previous indexeddb solutions dealt with it. Let me know if you come up with a simpler solution.

@harry-73
Copy link
Copy Markdown
Author

harry-73 commented Apr 24, 2025

I did a few tests. It seems that using EJSON.toJSONValue and EJSON.fromJSONValue preserves the ObjectId type and could be more generic.

@harry-73
Copy link
Copy Markdown
Author

harry-73 commented May 9, 2025

One idea would be to apply it if the collection is known to have idGeneration: MONGO and not limit the depth.

I think it will not work as you can have collection withoutidGeneration: MONGO which will contain documents with ObjectID field.
EJSON.toJSONValue and EJSON.fromJSONValue work pretty well. The issue I have is with the index creation.

@harry-73
Copy link
Copy Markdown
Author

harry-73 commented May 9, 2025

I did not test with the jam:archive package. Just want to know if you're OK with the modification.

Fix Error: UnknownError: Attempt to delete range from database without an in-progress transaction

IDBObjectStore.clear() takes no arguments. Passing the extra parameter 'names' raises an error on Safari/WebKit/Ios
pauseObservers/resumeObservers patch and restore store.endUpdate for every store, but some entries in Meteor.connection._stores (from queued method collection names) don’t have endUpdate. Restoring store.endUpdate = store._originalEnd when _originalEnd was undefined left endUpdate non-callable and triggered the error.

Fix: Only patch/restore when the method exists: in pauseObservers, do it only if typeof store.endUpdate === 'function'; in resumeObservers, only if typeof store._originalEnd === 'function'. Stores without endUpdate are left unchanged so the TypeError no longer occurs.
fix: e.endUpdate is not a function
Fix clearAll function to use correct parameters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants